Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: use i18next-scanner v3 for better i18next compatibility #864

Merged
merged 1 commit into from
Jul 19, 2024
Merged

fix: use i18next-scanner v3 for better i18next compatibility #864

merged 1 commit into from
Jul 19, 2024

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Jul 18, 2024 •
edited
Loading

Follow-up on an issue introduced by LIBS-641

The problem I encountered:
When upgrading @dhis2/cli-app-scripts to v11.6.3 in the Data Visualizer app, i18n extraction broke with an error like i18nextInstance.services.pluralResolver.getSuffixes is not a function

The cause:

  • I did some digging and found that v4 of i18next-scanner introduced the use of this API, which is only available in i18next v20+ (although this change was not really documented, and because of i18next-scanner's use of "*" for the version on their i18next dependency, maybe they're not even aware of it)
  • i18next-scanner's use of "*" for the version on their i18next dependency continues to prove problematic:
    • As a recap of the last issue, with i18next-scanner v2, when an app installed a new version of @dhis2/cli-app-scripts, it installed the latest version of i18next (v23), which i18next-scanner couldn't handle — this broke how plurals were parsed when extracting i18n strings
    • Also, though, when upgrading @dhis2/cli-app-scripts, the i18next version is left alone, which caused the issue described at the top
      • The Data Visualizer, for example, is on i18next v19, and upgrading @dhis2/cli-app-scripts doesn’t affect it
      • Then, when i18next-scanner v4 tried to use the i18next v20 API, it broke

Possible solutions:

  1. We could try to upgrade i18next in our libraries to make sure its >v20, but I'm not sure this would work... i.e., would i18next-scanner keep using whatever i18next version was installed when @dhis2/cli-app-scripts was first installed, and we end up with one more version of i18next in the dependencies?
    • Here are our current libraries and dependencies:
      • cli-app-scripts > i18next-scanner (used for build-time string extraction) & d2-i18n (used for runtime translation)
        • i18next-scanner > i18next @ *
        • d2-i18n > i18next @ ^10
    • Bumping d2-i18n 13 major versions should take some consideration, I think
  2. We could downgrade i18next-scanner to v3 in the CLI here: [This is my proposal, and what I did in this PR]
    • I tested this out, and it works in the Data Visualizer
    • This fixes our immediate problem; later, i18next-scanner and i18next can be upgraded when we make the full effort to support the new translation format

tomzemp
tomzemp approved these changes Jul 19, 2024
View reviewed changes
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's crummy 🫠

I approve as I think this is the least disturbing for people (prevent crash on running yarn start)

  • I guess we don't really know if i18next-scanner v3 is also making use of some breaking feature in certain i18next versions, so if one of our apps has a very old install of cli-app-scripts/old i18next-scanner, I guess we can run into this issue again 🙃 (maybe we should ask i18next-scanner team about this * dependency?)
  • it's a bit disappointing that we're falling back to i18next-scanner v3 when we know v4 was release because of a bug 🫤. I guess the specific bug isn't really relevant to how we do translations, but slightly annoying.
  • I agree that we should update the i18next dependency in d2-i18n, though also think that cli-app-scripts>i18next-scanner dependency will not pick that update up, and it seems we can't resolve to common i18next version as we don't use d2-i18n in cli? It does seem that even though we're really behind in 18next version in d2-i18n that it's mostly functionality we don't use, so hopefully it wont be a problem 🤞

For what it's worth simply upgrading @dhis2/cli-app-scripts does not work for me anyway as I also have to upgrade the @dhis2/app-runtime package to get the app to work (and also remove the existing shell). I usually just remove/reinstall cli-app-scripts as I find that it's the easiest to make sure dependencies update properly, but maybe that's a bit of an antipattern to suggest that 😄

@KaiVandivier KaiVandivier merged commit 84a5a59 into master Jul 19, 2024
16 checks passed
@KaiVandivier KaiVandivier deleted the libs-641-use-i18next-scanner-v3-fix branch July 19, 2024 11:04
dhis2-bot added a commit that referenced this pull request Jul 19, 2024
@dhis2-bot
chore(release): cut 11.6.4 [skip release]
47c9dd1 
## [11.6.4](v11.6.3...v11.6.4) (2024-07-19)

### Bug Fixes

* use i18next-scanner v3 for better i18next compatibility ([#864](#864)) ([84a5a59](84a5a59))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 11.6.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@KaiVandivier
Copy link
Contributor Author

KaiVandivier commented Jul 19, 2024 •
edited
Loading

Thanks for the thoughtful feedback! 🙏

Yeah I agree this isn't ideal, for those reasons you described 🙃

For upgrading cli-app-scripts, I usually run this, which handles the duplicates:

yarn add -D @dhis2/cli-app-scripts
npx yarn-deduplicate yarn.lock && yarn

Here's also some notes I have about if adding/updating our own i18next dependencies somewhere will help resolve i18next-scanner's:

I’m kinda unfamiliar with some of the details of how dependencies get resolved… like, the example I was wondering about above, if you have this situation:

  • cli-app-scripts dependencies:
    • i18next @ ^v21 (just for example — we would probably do >v20 if we wanted to keep i18next-scanner v4 and fix the extraction bug)
    • i18next-scanner @ v3 > i18next @ *
    • d2-i18n > i18next @ 10

Then what happens in...

Scenario 1: Data Visualizer w/ yarn why i18next: starting with i18next v10 for d2-i18n, and i18next v19 for i18next-scanner in yarn.lock

  1. Upgrade cli-app-scripts via yarn add -D @dhis2/cli-app-scripts (it already exists in dev dependencies)
  2. Do we end up with:
    1. A version for each package (i.e., * doesn’t care about versions used by other packages, either ahead or behind — it doesn’t seem to care about the v10 version from d2-i18n 🤷‍♂️)
      1. i18next @ v10 for d2-i18n
      2. i18next @ v21 for cli-app-scripts
      3. i18next @ v19 for i18next-scanner
    2. Or a consolidated version
      1. i18next @ v10 for d2-i18n
      2. i18next @ v21 for cli-app-scripts AND i18next-scanner

Scenario 2: Dashboard starting w/ i18next v10 for d2-i18n and i18next v23 for i18next-scanner from a recent installation of cli-app-scripts (this isn’t actually the case right now, but it was described by the analytics team as the reason why the set a root dependency on i18next@v20 for the app)

  1. Basically, same question — do you end up with a separate dependency for cli-app-scripts:
    1. i18next v10 for d2-i18n
    2. i18next v23 for i18next-scanner
    3. i18next v21 for cli-app-scripts
  2. Or would i18next downgrade to use a single version with cli-app-scripts:
    1. i18next v10 for d2-i18n
    2. i18next v21 for cli-app-scripts AND i18next-scanner

And unfortunately, it’s not a convenient thing to test, since in an app, you can’t really modify a dependency’s package.json in node_modules and see the effects in the same way that you could modify other files in node_modules… you have to publish a release

Right now, something I could investigate in the Dashboard: it currently has a root dependency on i18next ^v20 from the app to avoid v21 breaking plurals, which somehow ends up right now as:

  • i18next @ v10 for d2-i18n
  • i18next @ v20 for the local dependency and i18next-scanner

I say “somehow”, because when I uninstall and reinstall cli-app-scripts @v11.6.2 (before the recent plural “fix”), it ends up with

  • i18next @ v10 for d2-i18n
  • i18next @ v20 for Dashboard root
  • i18next @ v23 for i18next-scanner 🙃

…and plurals break again


So maybe setting i18next dependencies in the libraries won’t be a definitive fix… but on the other hand, maybe i18next will upgrade from v19 in the Data Visualizer case, but not downgrade in the Dashboard case? I don't know how to figure that out without publishing a test version

dhis2-bot added a commit that referenced this pull request Jul 23, 2024
@dhis2-bot
chore(release): cut 12.0.0-alpha.4 [skip release]
197f58a 
# [12.0.0-alpha.4](v12.0.0-alpha.3...v12.0.0-alpha.4) (2024-07-23)

### Bug Fixes

* use i18next-scanner v3 for better i18next compatibility ([#864](#864)) ([84a5a59](84a5a59))
* **deps:** update i18next-scanner version to support old plurals format again ([#861](#861)) ([d0e433b](d0e433b))
* plugin boundary retry if plugin logic is skipped ([#862](#862)) ([01a3160](01a3160))

### Features

* update boilerplate app code for init command [LIBS-644] ([#866](#866)) ([bd6cfc0](bd6cfc0))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 12.0.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomzemp tomzemp tomzemp approved these changes

Assignees
No one assigned
Labels
released on @alpha released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KaiVandivier @dhis2-bot @tomzemp